Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataProvider support, async fixes, headless AIR output #164

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jgranick
Copy link
Contributor

@jgranick jgranick commented Apr 4, 2019

This pull request merges DataProvider support, async test fixes and other improvements from the TiVo pull request here: #58

It also merges headless Adobe AIR support from here: #159

This improves upon the previous pull request (#161)


I have not included a rebuild of "index.n" or "run.n" in case this makes it easier to merge.

I have removed the massive.munit.Async class that was added in the original TiVo pull request and have changed it back to the original factory:AsyncFactory pattern in order to prevent breaking the API.

@jgranick
Copy link
Contributor Author

jgranick commented Apr 8, 2019

This has been updated with minor fixes. Thanks guys 😄

Copy link
Contributor

@elsassph elsassph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a significant PR!

});
runThread.sendMessage(Thread.current());
Thread.readMessage(true);
#if (!lime && !nme && (neko || cpp || java))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason to exclude lime and nme?
Could it be enabled by a more non project-specific directive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't see these comments!

I believe this code is to prevent applications which have no main loop from exiting immediately.

I agree it would be nice if this was a little more generic, but there's no certain way to say that you're using a framework that is aware of munit and can provide a main loop. I guess an munit-specific define that needs to be added would be a fair compromise

override public function print(value:Dynamic)
{
super.print(value);
#if(neko || cpp || java || cs || python || php || hl || eval)
Sys.print(value);
#elseif (flash && air)
flashOutput += value;
while (flashOutput.indexOf("\n") > -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not tracing as-is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the issue is that Flash trace supports only one line at a time

{
actualValue *= L1_RESPONS_FACTOR;

t2 = Async.handler ( this, forbiddenResponseHandler, 250, failingTimeOutHandler );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the asymetry between factory.createHandler (explicit) and Async.handler (implicit): could we instead have a factory.createAsyncHandler which would be passed the factory reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I thought I removed the Async.handler cases. Your suggestion sounds like a good compromise

I think the goal (originally) was to allow switching from sync to async without having to add an additional argument for the factory, but that change broke backward compatibility and prevented the pull request from ever being merged. I'm fine with leaving the factory argument in to get some of the other fixes/improvements we need

@elsassph
Copy link
Contributor

@jgranick sorry a recent merge is causingf a conflict - probably nothing serious.

@jgranick
Copy link
Contributor Author

jgranick commented Nov 8, 2019

I just resolved the conflict, thanks 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants